HCRC-97 | feat: image and image gallery modules #183
Conversation
|
Tested with Pixel phone in Storybook |
There was a problem hiding this comment.
The Pixel Phone view for Lightbox now fully fits to the screen, but it is quite useless like this
I think it is too small also for example for the iPad:
Sure the app can totally disable the light box on handhelds, but iPad for example should have quite high resolution, so I think it would be better like how it was earlier, so that it takes the full screen, but just so that it all fits to the screen.
I also added quite many code quality related comments. I just feel that they would make the code easier to understand and maintain, since there is quite much code needed for the features. The component does what it should and it now works, but I would just improve the code readability and reusability a bit.
Anyway, good work so far!
src/core/imageGallery/Lightbox.tsx
Outdated
| <div className={styles.actionsWrapper}> | ||
| <Button | ||
| iconLeft={<IconAngleLeft />} | ||
| onClick={onPreviousClick} | ||
| theme="black" | ||
| variant="secondary" | ||
| > | ||
| <span className={styles.screenReaderText}>{previous}</span> | ||
| </Button> | ||
| <Button | ||
| iconLeft={<IconAngleRight />} | ||
| onClick={onNextClick} | ||
| theme="black" | ||
| variant="secondary" | ||
| > | ||
| <span className={styles.screenReaderText}>{next}</span> | ||
| </Button> | ||
| </div> | ||
| <button | ||
| className={styles.closeButton} | ||
| id={`close-${lightboxUid}`} | ||
| type="button" | ||
| aria-controls={lightboxUid.toString()} | ||
| aria-expanded="true" | ||
| onClick={onClick} | ||
| > | ||
| <span className={styles.screenReaderText}> | ||
| {closeButtonLabelText} | ||
| </span> | ||
| <svg viewBox="0 0 24 24" aria-hidden="true" tabIndex={-1} /> | ||
| </button> |
There was a problem hiding this comment.
I would self use some compound component pattern for these, since the component is becoming quite big. Just something to think about.
| <div className={styles.actionsWrapper}> | |
| <Button | |
| iconLeft={<IconAngleLeft />} | |
| onClick={onPreviousClick} | |
| theme="black" | |
| variant="secondary" | |
| > | |
| <span className={styles.screenReaderText}>{previous}</span> | |
| </Button> | |
| <Button | |
| iconLeft={<IconAngleRight />} | |
| onClick={onNextClick} | |
| theme="black" | |
| variant="secondary" | |
| > | |
| <span className={styles.screenReaderText}>{next}</span> | |
| </Button> | |
| </div> | |
| <button | |
| className={styles.closeButton} | |
| id={`close-${lightboxUid}`} | |
| type="button" | |
| aria-controls={lightboxUid.toString()} | |
| aria-expanded="true" | |
| onClick={onClick} | |
| > | |
| <span className={styles.screenReaderText}> | |
| {closeButtonLabelText} | |
| </span> | |
| <svg viewBox="0 0 24 24" aria-hidden="true" tabIndex={-1} /> | |
| </button> | |
| <Lightbox.Actions/> | |
| <Lightbox.CloseButton/> |
Description
Issues
Closes
DEV-XXX:
Related
Testing
Automated tests
Manual testing
Screenshots
Additional notes